Skip to content

Conversation

@domdomegg
Copy link
Member

Summary

This PR relaxes the Accept header validation for servers with is_json_response_enabled=True. These servers only return application/json responses and never use SSE, so they should only require application/json in the Accept header rather than requiring both application/json and text/event-stream.

Changes

  • Modified _handle_post_request to conditionally validate the Accept header based on response mode
  • Extracted validation logic into a new _validate_accept_header method to reduce complexity
  • Added test coverage for JSON-only Accept header validation
  • Maintained backward compatibility - requests with both content types still work

Benefits

  • Easier to test JSON-only MCP servers with tools like curl
  • Follows the principle that servers should only require what they actually need to return
  • All existing tests pass

When is_json_response_enabled is True, servers only return application/json
responses and never use SSE. This change relaxes the Accept header validation
to only require application/json in this mode, rather than requiring both
application/json and text/event-stream.

This makes it easier to test JSON-only MCP servers with tools like curl,
which is useful when developing and debugging MCP servers.

For servers with is_json_response_enabled=False (SSE mode), the existing
requirement for both content types is maintained.
@domdomegg domdomegg requested a review from maxisbey October 20, 2025 19:06
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add tests that also cover the failure cases of this new validation?

E.g. missing Accept header and incorrect accept header (e.g. sending event/stream on a JSON server)

I think these should return 406

@felixweinberger felixweinberger added enhancement New feature or request needs more work Not ready to be merged yet, needs additional changes. labels Oct 23, 2025
Add tests to verify that JSON-only servers properly reject requests with:
- Missing Accept header (returns 406)
- Incorrect Accept header like text/event-stream only (returns 406)

These tests address the validation failure cases requested in the PR review
to ensure proper error handling for JSON-only server configurations.
@domdomegg
Copy link
Member Author

@felixweinberger sounds good, have added tests for those

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks allowed by the spec, though indirectly: https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#sending-messages-to-the-server

The client MUST include an Accept header, listing both application/json and text/event-stream as supported content types.

So technically speaking we should never even get into a situation where this is actually a problem as clients should always support both. However, JSON only servers are clearly allowed by this:

If the input is a JSON-RPC request, the server MUST either return Content-Type: text/event-stream, to initiate an SSE stream, or Content-Type: application/json, to return one JSON object. The client MUST support both these cases.

So relaxing the server-side requirement makes sense to me, though the benefit seems unclear right now. I guess we save a few keystrokes on curl not having to type both types?

@felixweinberger felixweinberger merged commit 31ae5f4 into main Oct 27, 2025
21 checks passed
@felixweinberger felixweinberger deleted the adamj/relax-accept-header-json-response branch October 27, 2025 17:46
@felixweinberger felixweinberger removed the needs more work Not ready to be merged yet, needs additional changes. label Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants